Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add local-telemetry stack for investigating server performance #16483

Merged
merged 7 commits into from
Dec 31, 2024
Merged

Conversation

zzstoatzz
Copy link
Collaborator

@zzstoatzz zzstoatzz commented Dec 23, 2024

related to #16299

this doesn't make any performances changes yet, instead it sets up a local telemetry stack we can use to explore OTEL data in Jaeger, which should make it easier to support / confirm suspected performance issues related to that issue

Copy link

codspeed-hq bot commented Dec 23, 2024

CodSpeed Performance Report

Merging #16483 will not alter performance

Comparing otel-oss (6e75593) with main (0049987)

Summary

✅ 3 untouched benchmarks

Comment on lines 52 to 55
if not os.environ.get("PREFECT__ENABLE_OSS_TELEMETRY"):
import prefect.telemetry.bootstrap

prefect.telemetry.bootstrap.setup_telemetry()
prefect.telemetry.bootstrap.setup_telemetry()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the short circuit that you need be moved to setup_telemetry or prefect.telemetry.bootstrap instead of being in prefect.main?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm i had intentionally guarded the imports as well but let me see if i can move it inside

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that the env var you're checking for will be unset in the large majority of cases, so it'd be better to be confined to that module. What's the reason for not importing that module if this environment variable is set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes sense, i was just under the impression that the imports themselves for the existing OTEL setup would interfere with the new local telemetry. but i haven't checked yet so that might not be true - will update

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@desertaxle because im targeting create_app directly

PREFECT_API_URL=http://localhost:4200/api \
OTEL_SERVICE_NAME=prefect-server \
OTEL_TRACES_EXPORTER=otlp \
OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4317 \
OTEL_EXPORTER_OTLP_PROTOCOL=grpc \
OTEL_LOG_LEVEL=debug \
PYTHONPATH=/Users/nate/github.com/prefecthq/prefect/src \
  opentelemetry-instrument \
  uvicorn \
  --app-dir /Users/nate/github.com/prefecthq/prefect/src \
  --factory prefect.server.api.server:create_app \
  --host 127.0.0.1 \
  --port 4200 \
  --timeout-keep-alive 5

I actually don't need this short circuit at all, so I removed it

Comment on lines +3 to +5
prefect --no-prompt work-pool create local --type process --overwrite

prefect --no-prompt deploy --all --prefect-file load_testing/prefect.yaml
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

populate the server

src/prefect/main.py Outdated Show resolved Hide resolved
PYTHONPATH=/Users/nate/github.com/prefecthq/prefect/src \
opentelemetry-instrument \
uvicorn \
--app-dir /Users/nate/github.com/prefecthq/prefect/src \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need to make the app-dir configurable or use a relative path.

load_testing/run-server.sh Outdated Show resolved Hide resolved
load_testing/README.md Outdated Show resolved Hide resolved
@zzstoatzz
Copy link
Collaborator Author

good catches @desertaxle - made those changes

load_testing/README.md Outdated Show resolved Hide resolved
@zzstoatzz zzstoatzz merged commit 307b0aa into main Dec 31, 2024
39 checks passed
@zzstoatzz zzstoatzz deleted the otel-oss branch December 31, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants